Skip to content

Fix azd issue #393 - #4567

Merged
vhvb1989 merged 7 commits into
Azure:mainfrom
v-weiyding:fix-issue#393
Dec 6, 2024
Merged

Fix azd issue #393#4567
vhvb1989 merged 7 commits into
Azure:mainfrom
v-weiyding:fix-issue#393

Conversation

@v-weiyding

@v-weiyding v-weiyding commented Nov 19, 2024

Copy link
Copy Markdown
Contributor

Fix #393.

Add code to the CheckInstalled function in the docker.go file to check if Docker is running.
The latest error information is shown below:
image

@rajeshkamal5050 for notification.

Comment thread cli/azd/pkg/tools/docker/docker.go

@vhvb1989 vhvb1989 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small fix requested. otherwise LGTM

Comment thread cli/azd/pkg/tools/docker/docker.go Outdated
@v-weiyding
v-weiyding requested a review from JeffreyCA as a code owner December 6, 2024 01:44
@vhvb1989
vhvb1989 merged commit e185e07 into Azure:main Dec 6, 2024
ellismg added a commit to ellismg/azure-dev that referenced this pull request Dec 9, 2024
When running `azd restore` for a Docker based project, we were
requiring that the docker tool itself was installed instead of
whatever tool would be used by the actual source project. This has two
implications:

1. If you didn't have the tool that was going to be invoked during
`restore` installed (i.e. you didn't have `npm` for a node based
project), we wouldn't detect the fact upfront and provide a nice
error, we'd fail later when we tried to invoke the tool.

2. As of Azure#4567, we now require that the docker daemon be running to
consider docker "installed" (which is reasonable for cases where we
are going to invoke docker commands) and so `azd restore` now fails if
you have a docker based project but are not running the docker deamon
(as is the case in the macOS environment in CI).

To fix both of these issues, I decided to accept the fact that
ensuring the tools needed to do a restore is different from the other
cases, and so the `ProjectManager` now has a `EnsureRestoreTools`
method. As part of this, I also changed the `ProjectManager` such that
`Initialize` no longer calls `EnsureAllTools` (which is logically did
before, it had just inlined the logic) and audited all the callers to
ensure that they were calling `EnsureAllTools` themselves or another
`EnsureXXXTools` variant (doing so also made me realize a lot of the
care we had put into some of the commands controling what sets of
tools they checked for via these special Ensure methods was being
undone by the implict call to `EnsureAllTools` from `Initialize`.

Fixes Azure#4612
ellismg added a commit to ellismg/azure-dev that referenced this pull request Dec 9, 2024
When running `azd restore` for a Docker based project, we were
requiring that the docker tool itself was installed instead of
whatever tool would be used by the actual source project. This has two
implications:

1. If you didn't have the tool that was going to be invoked during
`restore` installed (i.e. you didn't have `npm` for a node based
project), we wouldn't detect the fact upfront and provide a nice
error, we'd fail later when we tried to invoke the tool.

2. As of Azure#4567, we now require that the docker daemon be running to
consider docker "installed" (which is reasonable for cases where we
are going to invoke docker commands) and so `azd restore` now fails if
you have a docker based project but are not running the docker deamon
(as is the case in the macOS environment in CI).

To fix both of these issues, I decided to accept the fact that
ensuring the tools needed to do a restore is different from the other
cases, and so the `ProjectManager` now has a `EnsureRestoreTools`
method. As part of this, I also changed the `ProjectManager` such that
`Initialize` no longer calls `EnsureAllTools` (which is logically did
before, it had just inlined the logic) and audited all the callers to
ensure that they were calling `EnsureAllTools` themselves or another
`EnsureXXXTools` variant (doing so also made me realize a lot of the
care we had put into some of the commands controling what sets of
tools they checked for via these special Ensure methods was being
undone by the implict call to `EnsureAllTools` from `Initialize`.

Fixes Azure#4612
ellismg added a commit to ellismg/azure-dev that referenced this pull request Dec 11, 2024
When running `azd restore` for a Docker based project, we were
requiring that the docker tool itself was installed instead of
whatever tool would be used by the actual source project. This has two
implications:

1. If you didn't have the tool that was going to be invoked during
`restore` installed (i.e. you didn't have `npm` for a node based
project), we wouldn't detect the fact upfront and provide a nice
error, we'd fail later when we tried to invoke the tool.

2. As of Azure#4567, we now require that the docker daemon be running to
consider docker "installed" (which is reasonable for cases where we
are going to invoke docker commands) and so `azd restore` now fails if
you have a docker based project but are not running the docker deamon
(as is the case in the macOS environment in CI).

To fix both of these issues, I decided to accept the fact that
ensuring the tools needed to do a restore is different from the other
cases, and so the `ProjectManager` now has a `EnsureRestoreTools`
method. As part of this, I also changed the `ProjectManager` such that
`Initialize` no longer calls `EnsureAllTools` (which is logically did
before, it had just inlined the logic) and audited all the callers to
ensure that they were calling `EnsureAllTools` themselves or another
`EnsureXXXTools` variant (doing so also made me realize a lot of the
care we had put into some of the commands controling what sets of
tools they checked for via these special Ensure methods was being
undone by the implict call to `EnsureAllTools` from `Initialize`.

Fixes Azure#4612
ellismg added a commit that referenced this pull request Dec 11, 2024
When running `azd restore` for a Docker based project, we were
requiring that the docker tool itself was installed instead of
whatever tool would be used by the actual source project. This has two
implications:

1. If you didn't have the tool that was going to be invoked during
`restore` installed (i.e. you didn't have `npm` for a node based
project), we wouldn't detect the fact upfront and provide a nice
error, we'd fail later when we tried to invoke the tool.

2. As of #4567, we now require that the docker daemon be running to
consider docker "installed" (which is reasonable for cases where we
are going to invoke docker commands) and so `azd restore` now fails if
you have a docker based project but are not running the docker deamon
(as is the case in the macOS environment in CI).

To fix both of these issues, I decided to accept the fact that
ensuring the tools needed to do a restore is different from the other
cases, and so the `ProjectManager` now has a `EnsureRestoreTools`
method. As part of this, I also changed the `ProjectManager` such that
`Initialize` no longer calls `EnsureAllTools` (which is logically did
before, it had just inlined the logic) and audited all the callers to
ensure that they were calling `EnsureAllTools` themselves or another
`EnsureXXXTools` variant (doing so also made me realize a lot of the
care we had put into some of the commands controling what sets of
tools they checked for via these special Ensure methods was being
undone by the implict call to `EnsureAllTools` from `Initialize`.

Fixes #4612
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Issue] Test if docker is running before calling docker commands

3 participants